-
Notifications
You must be signed in to change notification settings - Fork 445
Polish link underline in Darkfish CSS #1349
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This aims to make sure an underline in a link doesn't overlap with underscores in a method name.
| color: var(--link-color); | ||
| transition: color 0.3s ease; | ||
| text-decoration: underline; | ||
| text-underline-offset: 2px; /* Make sure it doesn't overlap with underscores in a method name. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer 3px to 2px but I'm not good at visual design...
Others should decide it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a design expert either, but I find the 3px version better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
| color: var(--link-color); | ||
| transition: color 0.3s ease; | ||
| text-decoration: underline; | ||
| text-underline-offset: 2px; /* Make sure it doesn't overlap with underscores in a method name. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer 3px to 2px but I'm not good at visual design...
Others should decide it...
| a { | ||
| color: var(--link-color); | ||
| transition: color 0.3s ease; | ||
| text-decoration: underline; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
text-decoration: underline isn't necessarily required, but most browsers have it in their default user agent stylesheets. This is an example of Chrome:

So, I've added it to prevent our confusion, e.g., "Why is text-underline-offset specified, regardless text-decoration is not?"
If we want to respect browsers that don't have text-decoration: underline by default, we can remove it (text-underline-offset is just ignored). 👌🏼 In this case, it might be helpful to add a code comment that describes the reason for not having text-decoration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a question. We're adding this underline offset to all links, but not every link is a method name.
Will this look okay for other links that aren't method names? Should we use a more specific selector to match just method names or is the visual consistency better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think underline offset is fine for every links.
Link text could be an URL or a filename that contains underscore. Appearance of these links also gets better.
Another example, GitHub's "Convert to draft" link and URL in pull request description text also has text-underline-offset: .2rem; // about 3.2px.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback. For your information, it's easy to restrict only links that have <code>, but it might be harder for other links, e.g.
<a href="..."><code>::check_modeline</code></a>
<a href="...">::check_modeline</a>/* This is applied into the first `<a>` only, not the second one. */
a:has(code) { text-underline-offset: 3px; }There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As GitHub does, we can also use the rem unit instead of px. rem has a relative value from the root font size, so it might be better to maintain.
Considering the favors on #1349 (comment), I'll adjust the value if no other concerns:
- Use
reminstead ofpx - Increase the offset value from
2pxto keep more space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've increased the text-underline-offset value from 2px to 0.2em via f730435. See the following screenshots to see the new value:
Indeed, 0.2em is interpreted as 3.2px since the <main> and <nav> have font-size: 16px (16px * 0.2 == 3.2px):
rdoc/lib/rdoc/generator/template/darkfish/css/rdoc.css
Lines 292 to 298 in 8d07737
main { flex: 1; display: block; margin: 3em auto; padding: 0 2em; max-width: 800px; font-size: 16px; rdoc/lib/rdoc/generator/template/darkfish/css/rdoc.css
Lines 151 to 153 in 8d07737
nav { font-family: var(--font-heading); font-size: 16px;
The reason that I chose the em unit instead of rem is because the Darkfish CSS doesn't specify font-size to :root or html elements (rem stands for "root em"). If this CSS has font-size for :root in the future, we may need to reconsider the em value at the same time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tompng @vinistock I've pushed the commit f730435. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
| a { | ||
| color: var(--link-color); | ||
| transition: color 0.3s ease; | ||
| text-decoration: underline; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!




This aims to make sure an underline in a link doesn't overlap with underscores in a method name.
For example, method names on the following page seem a bit hard to read because underscores in a method name are hidden in an underline:

https://ruby.github.io/rdoc/table_of_contents.html#methods
This PR improves it a bit:
2px:

3px:

Note: The
text-underline-offsetCSS property is widely available now.